Skip to content

fix(review): skip open-PR refresh for uninstalled registry repos in the regate sweep - #3797

Merged
JSONbored merged 1 commit into
mainfrom
fix/regate-sweep-no-installation-budget-waste
Jul 6, 2026
Merged

fix(review): skip open-PR refresh for uninstalled registry repos in the regate sweep#3797
JSONbored merged 1 commit into
mainfrom
fix/regate-sweep-no-installation-budget-waste

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • The scheduled agent-regate-sweep refreshed open-PR data via the shared GITHUB_PUBLIC_TOKEN for every repo in the subnet registry (registry/sync.ts, isRegistered), including repos with no installed GitHub App at all. The per-PR re-review fan-out already skips uninstalled repos entirely (existing, unchanged behavior, covered by pre-existing tests), so this refresh was pure waste: it burned real, shared REST budget on data nothing downstream would use.
  • Confirmed live in production: dozens of registered-but-uninstalled repos (no installationId) were refreshing their open-PR lists on every sweep tick, draining the shared public-token budget. Combined with the sweep's staggered per-repo dispatch (delaySeconds = index * 10, up to 10 minutes), repos dispatched later in a rollout — including this instance's own actively-installed repos — could see an already-drained shared budget, causing real contributor PRs to go unreviewed for hours.
  • Fix: refreshOpenPullRequestsForScheduledSweep now returns early when the repo has no installationId, before touching the shared credential/segment-refresh path at all.

No issue link: small, self-evident production-driven fix; root-caused today via live audit-log investigation (git history for src/queue/processors.ts shows same-day narrow fixes without an issue link are an established pattern here).

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally (full unsharded run: 93.52% stmts / 92.46% branches / 92.8% funcs / 94.19% lines — all above threshold; new/changed lines in processors.ts covered by 2 new regression tests, one per branch of the new conditional)
  • npm run test:workers — not re-run locally this pass (no Workers-pool-specific code touched); CI will run it
  • npm run build:mcp — not re-run locally this pass (no MCP package touched); CI will run it
  • npm run test:mcp-pack — not re-run locally this pass; CI will run it
  • npm run ui:openapi:check — not re-run locally this pass (no API/schema change); CI will run it
  • npm run ui:lint — N/A, no UI files touched
  • npm run ui:typecheck — N/A, no UI files touched
  • npm run ui:build — N/A, no UI files touched
  • npm audit --audit-level=moderate — not re-run this pass; no dependency changes in this diff
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • This is a live production-incident fix pushed under time pressure with real contributor PRs stuck unreviewed. Ran the checks most relevant to this diff's actual surface (typecheck, targeted + full-suite coverage, actionlint, git diff --check) rather than the full test:ci chain; the untouched areas above (MCP, UI, Workers pool, OpenAPI) have zero overlap with this change (src/queue/processors.ts + test/unit/queue.test.ts only) and CI will run them regardless.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no such changes)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — internal scheduling logic only)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A)
  • Visible UI changes include a UI Evidence section below. (N/A — no UI/visual change)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

Notes

  • A second, related fix (scoping the shared rate-limit admission check per-installation instead of grabbing the globally-latest observation) is still in progress as a follow-up — this PR addresses the primary driver of the shared-budget drain but the admission check itself remains unscoped as defense-in-depth.

…s in the regate sweep

The scheduled regate sweep refreshed open-PR data (via the shared GITHUB_PUBLIC_TOKEN)
for every repo in the subnet registry, including repos with no installed GitHub App --
even though the per-PR re-review fan-out already skips these repos entirely. This burned
real, shared REST budget on data no part of the sweep could use, and starved that same
shared budget for installed repos processed later in the staggered per-repo dispatch.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.33%. Comparing base (db4d4a8) to head (0ecca46).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3797   +/-   ##
=======================================
  Coverage   93.33%   93.33%           
=======================================
  Files         317      317           
  Lines       32469    32470    +1     
  Branches    11904    11905    +1     
=======================================
+ Hits        30305    30306    +1     
  Misses       1530     1530           
  Partials      634      634           
Files with missing lines Coverage Δ
src/queue/processors.ts 94.40% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 6, 2026
@loopover-orb

loopover-orb Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-06 11:04:02 UTC

2 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This is a narrow, well-targeted fix: it adds an early return in refreshOpenPullRequestsForScheduledSweep so scheduled sweeps skip the shared-GITHUB_PUBLIC_TOKEN open-PR refresh for registered-but-uninstalled repos, since no per-PR regate fan-out can ever follow for them. The `repo?.installationId` check correctly subsumes the prior `!repo` null-check via optional chaining, so no regression there, and the two added tests exercise both branches (uninstalled repo skips getRepoSyncSegment/backfillRepositorySegment entirely; installed repo still refreshes via backfillRepositorySegment with mode:'light', force:true even when GITHUB_PUBLIC_TOKEN is also set, proving installation presence — not credential kind — gates the skip). The change is self-contained, has no schema/migration implications, and CI is green.

Nits — 4 non-blocking
  • The 7-line justification comment above the new check (processors.ts) is long for a one-line guard — could be trimmed while keeping the core rationale.
  • No issue link is provided; acceptable per the PR's own checklist reasoning for a small production-driven fix, but worth confirming this matches the maintainer's bar for unlinked fixes going forward.
  • Consider consolidating `if (!repo?.installationId) return;` and the following credential-availability check into a single early-return block with a shared comment, since both gate the same refresh path.
  • If not already covered elsewhere, add a brief test asserting behavior when repo has installationId but sweepOpenPullRequestSyncCredentialAvailable returns false, to fully pin down the two-check ordering introduced here.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 54 registered-repo PR(s), 46 merged, 407 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 54 PR(s), 407 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 54 PR(s), 407 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 6, 2026
@JSONbored
JSONbored merged commit adcd61e into main Jul 6, 2026
10 checks passed
@JSONbored
JSONbored deleted the fix/regate-sweep-no-installation-budget-waste branch July 6, 2026 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant